fix(form-core): 🐛 File input field - Picking a different file does not trigger rerender (deep equality issue with File/Blob objects) #1939
Conversation
|
packages/form-core/src/utils.ts
Outdated
| * @private | ||
| */ | ||
| export function setBy(obj: any, _path: any, updater: Updater<any>) { | ||
| const isFile = updater instanceof File |
There was a problem hiding this comment.
React Native does not have File defined and will throw a ReferenceError. You cannot implement the fix this way.
There was a problem hiding this comment.
Oh. Truly. Good catch, thanks!
|
View your CI Pipeline Execution ↗ for commit df74f02
☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1939 +/- ##
==========================================
- Coverage 90.35% 89.25% -1.11%
==========================================
Files 38 48 +10
Lines 1752 1954 +202
Branches 444 498 +54
==========================================
+ Hits 1583 1744 +161
- Misses 149 189 +40
- Partials 20 21 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…trigger-rerender' of github-personal.com:joaoGabriel55/form into fix/file-input-field-picking-a-different-file-does-not-trigger-rerender
LeCarbonator
left a comment
LeCarbonator
left a comment
There was a problem hiding this comment.
It looks like the previous comment hasn't been addressed between the previous review and this review request.
The actual required changes don't look that bad, though. Perhaps I'll have some time to help out later this week with that.
| if (!path.length) { | ||
| return functionalUpdate(updater, parent) | ||
| return functionalUpdate( | ||
| isFile(updater) ? { file: updater, uuid: uuid() } : updater, |
There was a problem hiding this comment.
This isn't type safe. This wrongly asserts File field values as { file: File | (oldFile: File) => File, uuid: string } instead of
- actually running the updater
- Preserving the expected field value's structure
| } | ||
|
|
||
| export function evaluate<T>(objA: T, objB: T) { | ||
| if (objA instanceof File && objB instanceof File) { |
There was a problem hiding this comment.
It looks like the previous comment wasn't addressed, despite the request for another review. This will throw ReferenceErrors in environments where File is not present (such as React Native).
| const firstFile = new File(['first'], 'first.png', { type: 'image/png' }) | ||
| form.setFieldValue('avatar', firstFile) | ||
|
|
||
| const firstValue = form.state.values.avatar as { file: File; uuid: string } |
There was a problem hiding this comment.
These type assertions are a huge problem. It cannot stay.
Luckily, as long as evaluate properly handles File types, this whole uuid trickery won't be needed. Just make sure to add unit tests for a File comparison with evaluate. I believe the existing tests can be found in utils.spec.ts.
3 participants
Issue: #1932
🎯 Changes
✅ Checklist
pnpm test:pr.🚀 Release Impact